Closed
Bug 1274106
Opened 9 years ago
Closed 9 years ago
Intermittent e10s devtools/client/aboutdebugging/test/browser_tabs.js | Test timed out
Categories
(DevTools :: about:debugging, defect, P3)
DevTools
about:debugging
Tracking
(e10s+, firefox49 wontfix, firefox50 fixed, firefox51 fixed, firefox52 fixed)
RESOLVED
FIXED
Firefox 52
People
(Reporter: philor, Assigned: ochameau)
References
(Blocks 1 open bug)
Details
(Keywords: intermittent-failure)
Attachments
(2 files)
58 bytes,
text/x-review-board-request
|
jdescottes
:
review+
|
Details |
1.13 KB,
patch
|
jdescottes
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Updated•9 years ago
|
Priority: -- → P3
Comment 1•9 years ago
|
||
Alex, it looks like your test timed out on Ubuntu once.
Flags: needinfo?(poirot.alex)
Assignee | ||
Comment 2•9 years ago
|
||
I imagine there is bigger intermittent fish to fry.
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1274106&startday=2016-05-10&endday=2016-05-19&tree=trunk&includefiltertype=quicksearch&includefilterdetailsexcludeResolved=true&includefilterdetailsexcludeDisabled=false&includefilterdetailsquicksearch=1274106&includefilterdetailsnumbugs=0&includefilterdetailsresolvedIds=&excludefiltertype=quicksearch&excludefilterdetailsquicksearch=&excludefilterdetailsnumbugs=0&excludefilterdetailsresolvedIds=
Given the log failure, it seems to fail during tab close, over here:
http://mxr.mozilla.org/mozilla-central/source/devtools/client/aboutdebugging/test/browser_tabs.js#43
45 yield removeTab(newTab);
46 yield onTabsUpdate;
Also, given that it was a linux32 failure, it may just be due to the extra super slowness of it...
Flags: needinfo?(poirot.alex)
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Summary: Intermittent browser_tabs.js | Test timed out → Intermittent devtools/client/aboutdebugging/test/browser_tabs.js | Test timed out
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Reporter | ||
Comment 19•9 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #2)
> I imagine there is bigger intermittent fish to fry.
No doubt there were, but right now, other than the js::ProfileEntry::pc crash this is the most-frequent devtools failure.
Summary: Intermittent devtools/client/aboutdebugging/test/browser_tabs.js | Test timed out → Intermittent e10s devtools/client/aboutdebugging/test/browser_tabs.js | Test timed out
Assignee | ||
Comment 20•9 years ago
|
||
Assignee | ||
Comment 21•9 years ago
|
||
Ok.
Looks like we never receive the DOM mutation:
21:04:39 INFO - 36 INFO Entering test bound
21:04:39 INFO - 37 INFO opening about:debugging
21:04:39 INFO - 38 INFO Adding a new tab with URL: about:debugging#tabs
21:04:39 INFO - 39 INFO Waiting for event: 'load' on [object XULElement].
21:04:39 INFO - 40 INFO Got event: 'load' on [object XULElement].
21:04:39 INFO - 41 INFO Tab added and finished loading
21:04:39 INFO - 42 INFO Adding a new tab with URL: data:text/html,<title>foo</title>
21:04:39 INFO - 43 INFO Waiting for event: 'load' on [object XULElement].
21:04:39 INFO - 44 INFO Got event: 'load' on [object XULElement].
21:04:39 INFO - 45 INFO Tab added and finished loading
21:04:39 INFO - 46 INFO Console message: [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol." {file: "data:text/html,<title>foo</title>" line: 0}]
21:04:39 INFO - 47 INFO TEST-UNEXPECTED-FAIL | devtools/client/aboutdebugging/test/browser_tabs.js | Test timed out -
The two tabs, about:debugging and data: url are both opened, but the `yield onNewTab;` never resolves.
This is different from comment 2.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → poirot.alex
Assignee | ||
Comment 22•9 years ago
|
||
Comment hidden (Intermittent Failures Robot) |
Updated•9 years ago
|
Blocks: e10s-tests
tracking-e10s:
--- → +
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment 26•9 years ago
|
||
I have a guess that using the openNewForegroundTab and removeTab functions of https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm might fix this. I'll work up a patch and push it through try to test my theory.
Assignee | ||
Comment 27•9 years ago
|
||
Any luck? I'm willing to help here, but I can't reproduce locally and whenever I try to push some additional debug logs, it stops reproducing :x
If you are running out of time, do you want me to try replacing with BrowserTestUtils helpers?
Comment 28•9 years ago
|
||
Using BrowserTestUtils helpers didn't help by itself. 1 of 10 try attempts timed out. I have another try run in progress that removes the waitForMutation where, as you pointed out above, `yield onNewTab;` never resolves. That try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f0a4db6d44f6e6d72389ab1d5e113608d007f1c
Assignee | ||
Comment 29•9 years ago
|
||
Oh wait, there is something wrong about the background tab. I didn't realiazed that before, but the tab isn't opened in background.
Here is a failure screenshot:
https://public-artifacts.taskcluster.net/Q-8khELfT626No7r_W0qJA/0/public/test_info//mozilla-test-fail-screenshot_NQzCBx.png
We clearly see it staying in foreground, I can easily imagine mutation event being paused if the tab isn't visible...
I also see the tab being in foreground when running locally.
Did your timeout was the same than the one we have here?
Assignee | ||
Comment 30•9 years ago
|
||
It looks like browser_tabs.js doesn't pull addTab from devtools/client/aboutdebugger/test/head.js, but another one... So if you modified addTab directly it wasn't taken into account.
Also, I don't see any way to open background tab in BrowserTestUtils??
Comment 31•9 years ago
|
||
here is the try log failure where I used BrowserTestUtils to open (openNewForegroundTab) and close (removeTab) the test tab: https://archive.mozilla.org/pub/firefox/try-builds/twalker@mozilla.com-a354dd723ef32c50851282a2b9c9cf4616ec0a8b/try-linux/try_ubuntu32_vm_test-mochitest-e10s-devtools-chrome-1-bm141-tests1-linux32-build49.txt.gz
I'm confused. tab "foo" is in the foreground in the screen capture, is that not supposed to be the case?
Assignee | ||
Comment 32•9 years ago
|
||
We explicitely ask it to be a background tab here:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/aboutdebugging/test/browser_tabs.js#24
let newTab = yield addTab(TAB_URL, null, true);
Third argument to true:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/aboutdebugging/test/head.js#71
But it looks like this addTab helper is overloaded by some other one... which doesn't support such third argument. That may just be that.
Comment 33•9 years ago
|
||
ah, my mistake. my attempts to fix this then are really changing the test entirely from checking the background tab to checking a new foreground tab.
Is it possible this !/devtools/client/framework/test/shared-head.js listed in browser.ini for this test directory, with it's own addTab function, could be the problem?
Assignee | ||
Comment 34•9 years ago
|
||
Assignee | ||
Comment 35•9 years ago
|
||
(In reply to Tracy Walker [:tracy] from comment #33)
> Is it possible this !/devtools/client/framework/test/shared-head.js listed
> in browser.ini for this test directory, with it's own addTab function, could
> be the problem?
Yes, most likely. Here is a try to fix that and reuse the one from shared-head:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d2a9bd3516a
Comment 36•9 years ago
|
||
I re-triggered M-e10s dt1 ten times in your try run. That is the chunk this test is in. Let's see if any of them fail for this test.
Comment 37•9 years ago
|
||
Looks like you got it Alexandre. 10 and 10 more re-triggers with no failures in this test.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 39•9 years ago
|
||
\o/ finally!
Julian, I'm removing addTab/removeTab from about:debugging's head.js as it was already overloaded by the one from shared-head.js. While doing that I'm backporting about:debugging features to the common one.
Comment 40•9 years ago
|
||
mozreview-review |
Comment on attachment 8791376 [details]
Bug 1274106 - Ensure opening tabs in background from about:debugging test.
https://reviewboard.mozilla.org/r/78800/#review77458
Great find, thank you for the investigation and fix.
::: devtools/client/aboutdebugging/test/head.js:66
(Diff revision 1)
> document.querySelector(`[aria-controls="${panelId}"]`).click();
> return waitForMutation(
> document.querySelector(".main-content"), {childList: true});
> }
>
> function closeAboutDebugging(tab, win) {
Remove unused win argument. Update browser_service_workers_not_compatible.js which was passing a win argument to this method.
Attachment #8791376 -
Flags: review?(jdescottes) → review+
Updated•9 years ago
|
status-firefox50:
--- → affected
status-firefox51:
--- → affected
Comment hidden (mozreview-request) |
Assignee | ||
Comment 42•9 years ago
|
||
mozreview-review |
Comment on attachment 8791376 [details]
Bug 1274106 - Ensure opening tabs in background from about:debugging test.
https://reviewboard.mozilla.org/r/78800/#review77528
Comment 43•9 years ago
|
||
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8be679eee5cf
Ensure opening tabs in background from about:debugging test. r=jdescottes
Comment 44•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Comment hidden (Intermittent Failures Robot) |
Comment 46•9 years ago
|
||
bugherder uplift |
Flags: in-testsuite+
Comment 47•9 years ago
|
||
This is still happening on central and aurora on pushes after this landed.
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1274106&endday=2016-09-19&startday=2016-09-16&tree=trunk
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1274106&endday=2016-09-19&startday=2016-09-18&tree=mozilla-aurora
Status: RESOLVED → REOPENED
status-firefox49:
--- → wontfix
Flags: needinfo?(poirot.alex)
Resolution: FIXED → ---
Target Milestone: Firefox 51 → ---
Assignee | ||
Comment 48•9 years ago
|
||
Hum well, that wasn't easy to track down, but these new intermittents were highlighting a significant issue in about:debugging where tabs with the same name/title/url would be merged into a single one.
STR:
- open about:debugging
- open a new tab and load: data:text/html,fooo
- open another tab with the same url
You will see only two tabs in about:debugging, the two data:text/html,fooo are going to be merged :/
That ends up messing up with this test where the new tab will sometimes be merged with the first empty firefox tab.
Flags: needinfo?(poirot.alex)
Assignee | ||
Comment 49•9 years ago
|
||
I don't know why `key` usage has been introduced in bug 1188981,
but it was preventing tabs with the same title or url to be displayed
correctly in about:debugging.
Attachment #8792527 -
Flags: review?(jdescottes)
Assignee | ||
Comment 50•9 years ago
|
||
Comment 51•9 years ago
|
||
Comment on attachment 8792527 [details] [diff] [review]
Allow displaying items with the same name/url/title in about:debugging
Review of attachment 8792527 [details] [diff] [review]:
-----------------------------------------------------------------
(In reply to Alexandre Poirot [:ochameau] from comment #49)
> Created attachment 8792527 [details] [diff] [review]
> Allow displaying items with the same name/url/title in about:debugging
>
> I don't know why `key` usage has been introduced in bug 1188981,
> but it was preventing tabs with the same title or url to be displayed
> correctly in about:debugging.
The key property is apparently used by React to uniquely identify children of a list, and know which state should be assigned to which component (in case you the list and re-render for instance). All our target components are stateless, so I don't think this is needed here. (and if it was, it should allow to display two tabs targetting the same URL).
BTW, this will also solve Bug 1285962.
Attachment #8792527 -
Flags: review?(jdescottes) → review+
Comment 52•9 years ago
|
||
Note that using a key can also avoid unnecessary re-renders from React, but given the lists we are displaying at the moment in about:debugging, this is not a must have. I will open follow ups to add tests covering the use cases you found here and reintroduce keys once we have proper test coverage.
Assignee | ||
Comment 53•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/e682130bece76629eb0ca95908f8cc14b30d0f91
Bug 1274106 - Allow displaying items with the same name/url/title in about:debugging. r=jdescottes
Comment 54•9 years ago
|
||
bugherder |
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Comment 55•9 years ago
|
||
Please request Aurora/Beta approval on this when you get a chance.
Flags: needinfo?(poirot.alex)
Comment hidden (Intermittent Failures Robot) |
Assignee | ||
Comment 57•9 years ago
|
||
Comment on attachment 8792527 [details] [diff] [review]
Allow displaying items with the same name/url/title in about:debugging
Approval Request Comment
[Feature/regressing bug #]: bug 1188981
[User impact if declined]: any item in about:debugging (tabs, service workers, addons) with duplicated name are going to merged into just one item.
[Describe test coverage new/current, TreeHerder]: this is the main reason of this uplift. uplift that small fix to prevent various intermittent.
[Risks and why]: small change into about:debugging, restricted to about:debugging but affect all its panels.
[String/UUID change made/needed]: no
The second patch is test only and could be uplifted with that one, but it needs this patch to really get rid of this intermittent.
Flags: needinfo?(poirot.alex)
Attachment #8792527 -
Flags: approval-mozilla-beta?
Attachment #8792527 -
Flags: approval-mozilla-aurora?
Comment on attachment 8792527 [details] [diff] [review]
Allow displaying items with the same name/url/title in about:debugging
Fixes a regression in about:debugging found by an intermittent test failure, Aurora51+, Beta50+
Attachment #8792527 -
Flags: approval-mozilla-beta?
Attachment #8792527 -
Flags: approval-mozilla-beta+
Attachment #8792527 -
Flags: approval-mozilla-aurora?
Attachment #8792527 -
Flags: approval-mozilla-aurora+
Comment 59•9 years ago
|
||
bugherder uplift |
Comment 60•9 years ago
|
||
bugherder uplift |
Comment hidden (Intermittent Failures Robot) |
Updated•7 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•